Support LogAppend timestamps #1258
Conversation
zimin2000
force-pushed
the
zimin2000.logappend.time
branch
from
d93518f
to
e666501
Compare
|
Looks okay to me, I wonder who else can review it? |
| {nil, testMsg, 1, now.Add(time.Second)}, | ||
| {nil, testMsg, 2, now.Add(2 * time.Second)}, | ||
| }, []time.Time{now.Add(time.Second), now.Add(2 * time.Second)}}, | ||
| {V0_11_0_0, true, []testMessage{ |
There was a problem hiding this comment.
Should you include other supported versions here?
There was a problem hiding this comment.
I have adjusted the test to cover all versions for which format of fetch is different (from https://github.com/bai/sarama/blob/0a21d90df4f6266fdf28d603e5ef91f2426c362a/fetch_request.go#L141)
zimin2000
force-pushed
the
zimin2000.logappend.time
branch
from
ca55d05
to
e1eda41
Compare
|
@sam-obeid , @varun06 , how else I can help with the PR to make it through ? |
| @@ -355,10 +357,13 @@ func encodeKV(key, value Encoder) ([]byte, []byte) { | |||
| return kb, vb | |||
| } | |||
|
|
|||
| func (r *FetchResponse) AddMessage(topic string, partition int32, key, value Encoder, offset int64) { | |||
| func (r *FetchResponse) AddMessageWithTimestamp(topic string, partition int32, key, value Encoder, offset int64, timestamp time.Time, version int8) { | |||
There was a problem hiding this comment.
This and below are exported function and this change is going to be a breaking change. Let's see how we can handle them. @bai do you have an idea?
There was a problem hiding this comment.
You'll need to keep both exported functions, and make one delegate to the other!
There was a problem hiding this comment.
@varun06 , I think I did preserve the original API call reimplementing the original function using this one (https://github.com/Shopify/sarama/pull/1258/files/e1eda41894f763094e70b74f6dbf958c4cbbd315#diff-be942b2617e64413fb251f24e4bd4ea2R388). Same for AddRecord.
Am I missing something ?
There was a problem hiding this comment.
This name change gonna break for folks who are using exported method "AddMessage".
There was a problem hiding this comment.
The “AddMessage” method is still there (check the link I provided above). Also note that there are multiple tests that would have got broken otherwise.
There was a problem hiding this comment.
Apologies, Do we have AddRecord method also?
There was a problem hiding this comment.
Yes, “AddRecord” also kept.
There was a problem hiding this comment.
Cool Bean. Let's get it going then. @bai can we merge this please?
|
@zimin2000 there are couple breaking changes(exported function names are changed), we got bitten by last merge, so let's walk carefully now and find a holistic approach to handle these changes. |
|
Thanks for contributing! |
Currently sarama doesn't seem to properly support LogAppend type of timestamps. The patch is addressing this issue. The following was used is the reference for the change: